wallet: Require the recorded fingerprint before import - #57
BenWestgate wants to merge 1 commit into
Conversation
BenWestgate
added
area: cli
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eaf535bb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Contract clarification applied in b50321a: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6da1f2a416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Claude), posted at the maintainer's request. I wrote 5eaf535, so this is partly self-review.
Not ACKing 6da1f2a.
- 6da1f2a removes
identifier_origin/identifier_note, which the no-record path was built to show. If that's intended,invariants.md:16-18andmodel.md:232still promise it (agree with the Codex P2). If not, revert it. Either way #57 now differs from #28, which keeps them. - Policy: 6da1f2a is authored by
Codex Preflight <codex-preflight@localhost>, and AI_POLICY.md forbids agent authors. Its message also contains a literal\n\nand has no area prefix. 5eaf535 has aCo-Authored-By: Claudetrailer, which the same policy forbids. Squash-merge or reword. - Q:
create --existingimports an existing seed (timestamp 0) withrestore=False, so it skips the fingerprint gate. Should it passrestore=True? The Enter-if-none path keeps it usable.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review, posted at the maintainer's request.
Concept ACK 70a188a. The no-record identifier evidence is restored and matches #28 again.
One correctness item remains: ms32 create --existing supplies an existing seed but still reaches _initialize_wallet(..., restore=False), so it can import without the wallet-record gate. Treat --existing as a restore for wallet initialization.
|
Release-gate verification at current head |
|
One non-code release-gate item still remains despite the code ACK: the current PR history still contains |
|
Release-gate history check: the functional fix is ACKed at |
795ccdd to
53cd58b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
53cd58b to
dcc0d41
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Release-gate history follow-up: the earlier commit-policy blocker is now resolved. Current head |
|
Security fix-verification refresh at current head |
dcc0d41 to
e59ac57
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Release-gate ACK 37eef4d.
I re-verified the refreshed one-commit restore-authentication change on top of current #46. BitcoinCore.initialize() calls verify_identity() before _select() or any wallet RPC/mutation; the mismatch regression leaves the RPC call log empty; the explicit no-record fallback remains deliberate; and both ms32 wallet and ms32 create --existing keep the recovered fingerprint hidden until the independent record/no-record decision. All inline review threads are resolved.
Exact-head validation passed 34 focused identity/fingerprint/existing/timestamp regressions, 922 tests normally, 922 under python -O, strict mypy, Ruff check/format, correction-constant verification, all 57 frozen differential cases and git diff --check. I also reran both isolated real-Core checks with Bitcoin Core 32.0rc2: regtest passed and the main-chain smoke fixture passed, both reporting /Satoshi:32.0.0/.
No remaining code blocker from this review. Human order for this line is #42 → #46 → #57 → #80 → #81.
|
Agent exact-head verification on
No remaining code blocker found for the recorded-fingerprint-before-mutation finding. Human review order remains #42 → #46 → #57. |
|
Independent security-fix verification against the original verify-before-mutate finding, rechecked on current head
This verifies the release-gate accident-safety claim: a typed-record mismatch cannot select, unlock, create, or import into a Bitcoin Core wallet. It does not claim malicious-share-tampering resistance; #55 remains the stronger encrypted-descriptor design. |
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
37eef4d to
a7efaae
Compare
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
5ffb196 to
1435560
Compare
a7efaae to
054e8d9
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted security release-gate re-review: ACK 054e8d9.
The verify-before-mutate finding is fixed on this exact head. BitcoinCore.initialize() validates the recovered seed against the independently recorded fingerprint before _select() or any wallet RPC/mutation; the mismatch regression leaves the wallet call path untouched. The explicit no-record fallback remains available, while ms32 wallet and ms32 create --existing keep the recovered fingerprint hidden until the independent record/no-record decision. Focused exact-head checks for mismatch, no-record, hidden-fingerprint, and existing-seed record gating passed (5 tests).
This replay has the same stable patch-id b1be2d64… as the previously reviewed 37eef4d and a7efaae versions. Exact-head CI is green, including the Core fixture, and the stack remains within the strict review-size gate at 5,196 logical lines on top of #42.
No remaining security/code-review blocker found. Human stack order remains #42 → #57 → #105 → #80 → #81 → #95.
Gate restore and existing-seed wallet initialization on the independently recorded BIP32 master fingerprint before any Bitcoin Core wallet mutation. Keep the correction path from disclosing or reusing a fingerprint derived from the candidate being authenticated. Fixes #30.
054e8d9 to
115f2c2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
`ms32 wallet` and `ms32 create --existing` hid the recovered master fingerprint while confirming a correction (#57), so a wrong correction was caught only after the operator accepted it and typed the record. Ask for the record first: before the shares in `ms32 wallet` and before the seed in `ms32 create --existing`. A correction that completes the secret then says whether it matches the record, without showing the fingerprint, and the record picks between equally likely corrections. The final identity check, the retry on mismatch and the Enter path for no record work as before; without a record nothing is shown until the recordless gate. Ctrl-C at the moved prompt still says the existing cards are valid. Closes #91 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head release-gate re-review: ACK 115f2c2 as the restore-authentication stack unit.
The current diff still enforces the key invariant: BitcoinCore.initialize() calls verify_identity() before wallet selection or mutation; ms32 wallet and ms32 create --existing suppress recovered-fingerprint disclosure until the independent record/no-record decision; declining the recordless path terminates the attempt. All existing inline findings are resolved. Exact-head Python-package run 658 and Bitcoin Core wallet-fixture run 32 both succeeded.
Known downstream requirement: #80 must remain in the frozen stack because it corrects the no-record wording when RIPEMD-160 is unavailable. That does not weaken this PR's verify-before-mutate behavior. No new blocker found in this current diff; human review remains required before integration.
This branch has not been deployed
1 participant
Fixes #30. Library/CLI half of #26; GUI half is tracked separately.
This adds the restore-time wallet identity gate before Bitcoin Core mutation.
BitcoinCore.initialize()accepts an expected fingerprint and checks it before wallet selection, unlock, creation or import.ms32 walletrequires the fingerprint from the wallet record; mismatch retries without changing Core.ms32 create --existinguses the same restore gate and does not expose the recovered fingerprint through correction or rendering before the independent record/no-record decision;create --existingrecord/no-record decision earlier, immediately after parsing the existing seed and before any new share ceremony or output;ms32 createonly records the new fingerprint; it has no pre-existing wallet identity to authenticate;parse_fingerprint()accepts 8 hex digits in any case or spacing.#43 tracks checksummed wallet-record fields; #55 tracks the separately planned encrypted full-descriptor backup; #56 tracks identifier-assisted correction ranking. The current release gate is accident safety, not malicious-share-tampering resistance.
Review shape
Current head
054e8d9is the same Ben Westgate-authored restore-authentication patch replayed directly onto current #42 (128bda4). Its stable patch-id is identical to the previously revieweda7efaae/37eef4dpatch; the replay changes no restore behavior.The current #42 tip is 5,092 installed production logical lines. Adding this unchanged restore patch produces 5,196 lines, keeping the intermediate commit within the maintainer-authorized
<5200gate without raising the cap or taking a security-sensitive refactor.Security verification
BitcoinCore.initialize()callsverify_identity()before_select()or any wallet RPC/mutation;test_identity_mismatch_stops_before_any_wallet_callpasses and asserts the RPC call log stays empty on mismatch;test_no_record_is_the_operators_choice_and_checks_nothingpasses, preserving the explicit fallback contract;ms32 walletandms32 create --existing.Validation on
054e8d9python -O;git diff --check: clean;Human review/integration order for this stack is #42 → #57 → #105 → #80 → #81 → #95. #45/#46 are already contained in #42's head branch; #7/#51 are already in
reviewability-v1.AI assistance was used for the authorized mechanical rebase/conflict validation and review follow-ups. The focused source commit retains Ben Westgate as its author and still requires responsible human review before integration.